-
Notifications
You must be signed in to change notification settings - Fork 821
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(exporters): introduce packages for shared exporter classes #2893
refactor(exporters): introduce packages for shared exporter classes #2893
Conversation
…exporter-base package
…-exporter-base package
…tlp-exporter-base package
…tlp-exporter-base package
…otlp-exporter-base package
Codecov Report
@@ Coverage Diff @@
## main #2893 +/- ##
==========================================
- Coverage 93.73% 92.81% -0.93%
==========================================
Files 187 184 -3
Lines 6145 6066 -79
Branches 1308 1296 -12
==========================================
- Hits 5760 5630 -130
- Misses 385 436 +51
|
@pichlermarc hi, could you please provide an update on this? I wonder what's the ETA. Thx |
I'm not in the office this week, so I handed this PR over to @dyladan while I'm away. It is pretty much done though, but when I left some checks were failing. |
There is still some minor cleanup but i'll mark it as ready for review so we can get some 👀 on it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, since this one is huge i think it would be better to have more than 2 approvers to approve it for it to be merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 🙏 A few minor comments
experimental/packages/exporter-trace-otlp-http/test/node/CollectorTraceExporter.test.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-exporter-metrics-otlp-grpc/package.json
Outdated
Show resolved
Hide resolved
Hi team, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big PR and tough to see if there are small things to clean up but tests are passing and it LGTM generally.
What is the plan for the future for these exporters? do we plan to keep them separated or merge them into a single exporter per-protocol that can export all stable signals (trace, metrics, and logs in the future)? I don't see any good reason to separate them once metrics is stable, in which case we will probably need to merge these new packages back soon, right? |
As far as I know the exporters were split by signal in order to keep bundle size down. This was originally done by @obecny. Personally I think the developer experience is better to have a single exporter for all signals, but I'm not sure how much this affects the bundle size. Maybe @MSNev has some idea? |
I think this depends on if we want to support signals being used independently (see @dyladan's answer on bundle size). Regardless of that, I think keeping these new packages until the Logs SDK is stable is necessary if we want to keep experimental code in
I think we would have to split |
When it was used for example in aws lambda (if I remember correctly) you don't want to have many MB as there is already a limit for your function. The same applies for browser you don't want to load many unneeded MB or kB of code. So hence the decision to split them so the user can include only what is necessary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 this is a great job!
Even if we merged trace/metrics OTLP exporters into one stable package, I think in the case of reducing bundle size in certain environments, people could still use tools to tree-shaking unused code components. |
Which problem is this PR solving?
Currently the metrics exporters depend on the trace exporters, which causes unwanted dependencies to Trace packages via the Metrics exporters. This PR is part of the work to get rid of these dependencies by introducing new packages for Trace-independent export code that was previously exported from the
exporter-trace-otlp-grpc
,exporter-trace-otlp-http
andexporter-trace-otlp-proto
packages.These shared classes have been moved to
otlp-exporter-base
,otlp-grpc-exporter-base
andotlp-proto-exporter-base
. Therefore,opentelemetry-exporter-metrics-otlp-http
opentelemetry-exporter-metrics-otlp-proto
opentelemetry-exporter-metrics-otlp-grpc
exporter-trace-otlp-grpc
exporter-trace-otlp-http
exporter-trace-otlp-proto
have been updated to make use of these new packages instead.
While the exporters still depend on
exporter-trace-otlp-http
for transformation of the internal types to the proto-representation, this can be handled in a follow-up PR.Related Issue: #2886
Short description of the changes
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: